Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove NFT protocols for V4 router #348

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

hensha256
Copy link
Collaborator

Remove NFT protocols to prepare for a new version that supports uniswap v2, v3 and v4

@hensha256 hensha256 changed the title first pass remove NFT protocols for V4 router Jun 4, 2024
@hensha256 hensha256 marked this pull request as ready for review June 4, 2024 23:32
@@ -79,6 +60,6 @@ contract UniversalRouter is IUniversalRouter, Dispatcher, RewardsCollector {
return command & Commands.FLAG_ALLOW_REVERT == 0;
}

/// @notice To receive ETH from WETH and NFT protocols
/// @notice To receive ETH from WETH protocols
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can finally add this in now?

require(msg.sender == WETH9, 'Not WETH9');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's been a huge thorn in our side

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll probably need to check that it's either WETH or v4 PoolManager

expect(miladyBalanceAfter.sub(miladyBalanceBefore)).to.eq(numMiladys)
})
})
it('reverts if no commands are allowed to revert')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrun test cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeeah need to completely rewrite them as they were all written for NFTs

Copy link
Member

@ewilz ewilz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add that receive() external payable {} change???

@hensha256 hensha256 merged commit db41419 into use-mainnet-permit2 Jun 12, 2024
5 checks passed
@hensha256 hensha256 deleted the remove-nft-protocols branch June 12, 2024 14:32
hensha256 added a commit that referenced this pull request Jun 12, 2024
* solc upgrade to 0.8.26

* use mainnet permit2 work started

* fix uniswap tests

* Remove block from resetFork

* Refactor to fetch fee tiers

* remove NFT protocols for V4 router (#348)

* first pass

* fix forge builds

* fix reentrancy test

* Add check to receive
hensha256 added a commit that referenced this pull request Jun 14, 2024
* update hardhat

* remove symlinks by using hardhat-foundry

use hardhat-foundry package to use foundry remappings to compile. Symlinks are no longer necessary
After upgrade HH412 error was thrown caused by the existing symlinks
ref:
https://hardhat.org/hardhat-runner/docs/errors#HH412
NomicFoundation/hardhat#3623

* update compiler version to ^0.8.24 supporting the cancun upgrades

* fix ci

* fix lock file

* Update yarn.lock

* regenerate gas snapshots

* install foundry in ci

* Use mainnet permit2 (#347)

* solc upgrade to 0.8.26

* use mainnet permit2 work started

* fix uniswap tests

* Remove block from resetFork

* Refactor to fetch fee tiers

* remove NFT protocols for V4 router (#348)

* first pass

* fix forge builds

* fix reentrancy test

* Add check to receive

* remove .only rip

* add todo for tests that need wrtiting

* update readme and planner

---------

Co-authored-by: Alice <34962750+hensha256@users.noreply.github.com>
Co-authored-by: Alice Henshaw <henshawalice@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants